-
Notifications
You must be signed in to change notification settings - Fork 25
Implement getBuildArtifacts tool and optimize getBuild response #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add BuildArtifactsExtension with getBuildArtifacts and getBuildArtifact tools - Create RunWithoutArtifactsSerializer to exclude artifacts from getBuild responses - Add comprehensive tests for new functionality - Implement pagination support for large artifact files - Maintain backward compatibility while reducing payload sizes Addresses first part of issue jenkinsci#32: separate artifacts retrieval from build metadata
- Replace Java record with regular class for Java 11 compatibility - Fix regex pattern in RunWithoutArtifactsSerializer with DOTALL flag - Update test methods to use proper McpSchema.TextContent pattern - Remove non-existent TestUtils.getTextContent() method calls - Follow existing test patterns from the codebase
- Declare ObjectMapper inside lambda scope where it's used - Resolves compilation error in GetBuildWithoutArtifactsTest.java
- Use record for BuildArtifactResponse - Use TreePruner - spotless:apply
Working on the code coverage warnings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR.
the title says optimize getBuild response
. is it remove artifacts from the tree?
* - "unchecked": Run.getArtifacts() returns raw List requiring unchecked conversion | ||
*/ | ||
@SuppressWarnings({"rawtypes", "unchecked"}) | ||
@Tool(description = "Get the artifacts for a specific build or the last build of a Jenkins job") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need annotations = @Tool.Annotations(destructiveHint = false)
as it's true
per default
.orElse(List.of()); | ||
} | ||
|
||
@Tool( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need annotations = @Tool.Annotations(destructiveHint = false)
as it's true
per default
offset = 0L; | ||
} | ||
if (limit == null || limit <= 0) { | ||
limit = 65536; // 64KB default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be better having a configuration (e.g SystemProperties) as users may want to configure more or less.
} | ||
|
||
// Cap the limit to prevent excessive memory usage | ||
final int maxLimit = 1048576; // 1MB max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. should be configurable
} | ||
// Serialize all results the same way - this fixes the JSON concatenation issue | ||
// for top-level lists while maintaining proper JSON structure | ||
resultBuilder.addTextContent(toJson(result)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great. it's going to the direction of #82
} else { | ||
resultBuilder.addTextContent(toJson(result)); | ||
} | ||
// Serialize all results the same way - this fixes the JSON concatenation issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need of this comment here.
it;s more a PR comment :)
} | ||
|
||
// Convert to string (assuming text content) | ||
String content = new String(buffer, 0, bytesRead, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about binaries artifacts? (we can attach jar, war etc....)
This PR implements issue #32 by adding new tools for artifact management and optimizing the getBuild response size.
New Tools Added
getBuildArtifacts - Returns only the artifacts array from a build
o Takes jobFullName and optional buildNumber parameters
o Significantly reduces payload size compared to full getBuild response
o Returns empty array for jobs without artifacts or non-existent jobs
o Fixed JSON concatenation issue for top-level lists
getBuildArtifact - Provides paginated access to individual artifact file contents
o Supports offset/limit parameters for large files
o Returns artifact content as text with pagination metadata
o Includes safety limits to prevent excessive memory usage
Optimizations
o Created RunWithoutArtifactsSerializer to filter out artifacts
o Updated JenkinsExportedBeanSerializerModifier to use custom serializer
o Maintains all other build information while reducing response size
Testing done
Comprehensive test coverage
o BuildArtifactsExtensionTest - Integration tests for new tools
o GetBuildWithoutArtifactsTest - Verifies getBuild optimization
o BuildArtifactsExtensionCompileTest - Basic compilation verification
Manual test
o Augment Code MCP client w/
mvn hpi:run
- Found JSON issue with top-level listo Manual test with Augment Code and production Jenkins server
Submitter checklist